1463755 - Update the design of certificate error pages

Status: RESOLVED FIXED

(Firefox :: Security, enhancement, P3)

Description

[Trisha Gupta [:trisha]]

This bug is to update to the new design of certificate error pages. See user story for planned design and copy.

Comment 1

[Trisha Gupta [:trisha]]

Hi Bram, can you please attach the error icon and also let me know about the exact yellow color(border) specification so that I could add them to the design? Thanks :)

Comment 2

[Bram Pitoyo [:bram]]

Attachment: warning-broken-lock.svg

Comment 3

[Bram Pitoyo [:bram]]

The yellow border specification is as follows:

> border-style: solid;
> border-color: --yellow-50;
> border-width: 16px;

Comment 4

[Bram Pitoyo [:bram]]

Updated the user story with the link to the design mocks. Nothing else, other than the URL – has changed.

Comment 5

[Bram Pitoyo [:bram]]

Attachment: blue-berror.svg

I’m unfamiliar with the way our in-content SVG illustrations are exported. I hope this is the right approach – minified through SVGO. If not, please ask Sean Martell for the asset.

Comment 6

[Trisha Gupta [:trisha]]

Attachment: bug1463755.patch

This is for the certificate error pages except for the computer clock error pages. Please give feedback, Thanks!

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8986262 [details] [diff] [review]
bug1463755.patch

Review of attachment 8986262 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like the right general structure, but we need to make sure that the "Advanced" panel and the certificate info overflow correctly. I know that this is a bit tricky, so please feel free to chat me up on Slack so that we can figure this out together.

Thanks!

::: browser/themes/shared/aboutNetError-new.css
@@ +6,2 @@
>  
>  :root {

Please don't set your rules on :root and rather on something like

body.certerror

to be sure that we're only showing this on certificate errors...

@@ +12,5 @@
> +  position: absolute;
> +  width: 100%;
> +  height: 100%;
> +  box-sizing: border-box;
> +  overflow: hidden

Setting explicit height and using overflow:hidden here breaks the way that the certificate info is displayed when you click the "Advanced" button and/or click on the error code afterwards (because the #advancedPanelContainer is using position: absolute).

I think what we want to do is make the yellow border wrap around all the information on the page, also to signify to the user that there's more stuff to look at.

The problem is that the #errorPageContainer is currently centered using flexbox (with justify-content: center). That doesn't work anymore, because I don't think we can make the container grow correctly when it's using flex.

It's a little hacky, but how about removing the justify-content: center and position: absolute rules and just setting the upper margin for the #text-container using JS? So maybe like the following pseudo-code:

#text-container.margin-top = 50vh - (#text-container.clientHeight / 2)

::: browser/themes/shared/incontent-icons/cert-error.svg
@@ +1,1 @@
> +<svg height="96" viewBox="0 0 96 96" width="96" xmlns="http://www.w3.org/2000/svg"><g fill="none" fill-rule="evenodd"><path d="m54 87h-38.718c-4.1514504-.0042773-8.00579913-2.1539983-10.19089334-5.6838598-2.1850942-3.5298615-2.39050529-7.9383886-.54310666-11.6561402l29.718-59.46c2.0323569-4.06636891 6.1880314-6.63518463 10.734-6.63518463s8.7016431 2.56881572 10.734 6.63518463l19.0437959 38.0875919c-8.4035561 1.5187368-14.7777959 8.8711807-14.7777959 17.7124081v1.0104467c-3.547421 1.6851959-6 5.3009594-6 9.4895533z" fill="#ffe900" fill-rule="nonzero"/><path d="m39 27c0-3.3137085 2.6862915-6 6-6s6 2.6862915 6 6v24c0 3.3137085-2.6862915 6-6 6s-6-2.6862915-6-6zm6 49.5c-4.1421356 0-7.5-3.3578644-7.5-7.5s3.3578644-7.5 7.5-7.5 7.5 3.3578644 7.5 7.5-3.3578644 7.5-7.5 7.5z" fill="#3e2800"/><path d="m89.2954301 61.9390003c.4560585 1.2683141.7045699 2.6356354.7045699 4.0609997v6h1.5c2.4620372-.0002189 4.4671728 1.9781816 4.5 4.44v15c.0160526 1.203836-.4509571 2.3639032-1.296617 3.2208385-.8456598.8569353-1.99944 1.3392685-3.203383 1.3391615h-27c-2.4852814 0-4.5-2.0147186-4.5-4.5v-15c0-2.4852814 2.0147186-4.5 4.5-4.5h1.5v-6c0-6.627417 5.372583-12 12-12 2.9975478 0 5.7383932 1.0990736 7.8415359 2.9162204l-4.2654615 4.2654616c-.998662-.7424058-2.236069-1.181682-3.5760744-1.181682-3.3137085 0-6 2.6862915-6 6v6h12v-4.7655696z" fill="#b1b1b3" fill-rule="nonzero"/></g></svg>
\ No newline at end of file

This is currently overriding the existing cert-error.svg file, which makes it appear in the old cert error pages as well. That might actually be okay, considering that we're probably not landing this before the 63 merge, I just wanted to note it.

Also, can you please add line breaks after these tags and indentation, to clean it up a bit? So, like,

<svg ...>
  <g ...>
    <path ...>
    </path>
  </g>
</svg>

Comment 8

[Trisha Gupta [:trisha]]

Attachment: Bug 1463755 - Update the design of certificate error pages

Review commit: https://reviewboard.mozilla.org/r/253126/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253126/

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8987852 [details]
Bug 1463755 - Update the design of certificate error pages

https://reviewboard.mozilla.org/r/253126/#review260152

This looks great, just a few implementation details to correct.

Thank you!

::: browser/base/content/aboutNetError.js:185
(Diff revision 1)
>      initPageCaptivePortal();
>      return;
>    }
>    if (gIsCertError) {
>      initPageCertError();
> +    var textContainer = document.getElementById("text-container");

nit: please use let instead of var

::: browser/base/content/aboutNetError.js:186
(Diff revision 1)
>      return;
>    }
>    if (gIsCertError) {
>      initPageCertError();
> +    var textContainer = document.getElementById("text-container");
> +    textContainer.style.marginTop = `calc(50vh - ${textContainer.clientHeight / 2}px)`;

Since you removed the justify-content: center style from all error pages (not just cert error pages), you need to always apply this workaround, not just when gIsCertError is true.

::: browser/themes/shared/error-pages.css:11
(Diff revision 1)
>  
>  body {
>    background-size: 64px 32px;
>    background-repeat: repeat-x;
>    /* Top padding for when the window height is small.
>       Bottom padding to keep everything centered. */

you can remove this comment now

::: browser/themes/shared/error-pages.css:12
(Diff revision 1)
>  body {
>    background-size: 64px 32px;
>    background-repeat: repeat-x;
>    /* Top padding for when the window height is small.
>       Bottom padding to keep everything centered. */
> -  padding: 75px 0;
> +  padding: 0px 0;

you can just write

padding: 0

::: browser/themes/shared/error-pages.css:32
(Diff revision 1)
>    flex: 1;
>  }
>  
>  @media only screen and (max-width: 959px) {
>    body {
>      padding: 75px 48px;

I think you can remove this padding here.

::: browser/themes/shared/error-pages.css:52
(Diff revision 1)
>    body {
>      justify-content: unset;
>      /* Now that everything is top-aligned, we don't need the
>       * bottom padding for centering - though it's added back
>       * when the viewport height is < 480px (see below). */
>      padding: 75px 20px 0;

I think these two rules can also be unset.

::: browser/themes/shared/incontent-icons/cert-error.svg:1
(Diff revision 1)
> -<?xml version="1.0" encoding="utf-8"?>
> +<svg height="96" viewBox="0 0 96 96" width="96" xmlns="http://www.w3.org/2000/svg">

I think we should make a new file cert-error-new.svg instead of overwriting the old one.

::: browser/themes/shared/incontent-icons/cert-error.svg:2
(Diff revision 1)
> -<?xml version="1.0" encoding="utf-8"?>
> -
> +<svg height="96" viewBox="0 0 96 96" width="96" xmlns="http://www.w3.org/2000/svg">
> +	<g fill="none" fill-rule="evenodd">

please use two spaces indentation

Comment 10

[Trisha Gupta [:trisha]]

Comment on attachment 8987852 [details]
Bug 1463755 - Update the design of certificate error pages

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253126/diff/1-2/

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8987852 [details]
Bug 1463755 - Update the design of certificate error pages

https://reviewboard.mozilla.org/r/253126/#review261216

This looks great but please fix the mentioned nits before landing. Let me know if you have any questions!

Thank you!

::: browser/base/content/aboutNetError.js:298
(Diff revision 2)
>      var container = document.getElementById("errorLongDesc");
>      for (var span of container.querySelectorAll("span.hostname")) {
>        span.textContent = document.location.hostname;
>      }
>    }
> +    textContainerStyle();

nit: I think this has one level too much indentation

::: browser/base/content/aboutNetError.js:301
(Diff revision 2)
>      }
>    }
> +    textContainerStyle();
> +}
> +
> +function textContainerStyle() {

nit: this could use a little better name, something like verticallyCenterContainer, or updateContainerPosition?

::: browser/themes/shared/error-pages.css:31
(Diff revision 2)
>  }
>  
>  @media only screen and (max-width: 959px) {
>    body {
> -    padding: 75px 48px;
> +    padding: 0;
>    }

I think that you can remove this complete rule (lines 29 - 31). Can you please remove those and check whether it still looks fine on small window sizes?

::: browser/themes/shared/error-pages.css:46
(Diff revision 2)
>    }
>  }
>  
>  @media only screen and (max-width: 640px) {
>    body {
>      justify-content: unset;

I think you can remove this.

::: browser/themes/shared/error-pages.css:49
(Diff revision 2)
>  @media only screen and (max-width: 640px) {
>    body {
>      justify-content: unset;
>      /* Now that everything is top-aligned, we don't need the
>       * bottom padding for centering - though it's added back
>       * when the viewport height is < 480px (see below). */

I think you can remove this comment.

::: browser/themes/shared/error-pages.css:51
(Diff revision 2)
>      justify-content: unset;
>      /* Now that everything is top-aligned, we don't need the
>       * bottom padding for centering - though it's added back
>       * when the viewport height is < 480px (see below). */
> -    padding: 75px 20px 0;
> +    padding: 0;
>    }

It's probably better to retain some horizontal padding for very small window widths, so I suggest making this:

padding: 0 75px;

Comment 12

[Trisha Gupta [:trisha]]

Comment on attachment 8987852 [details]
Bug 1463755 - Update the design of certificate error pages

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253126/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2835f9a11445
Update the design of certificate error pages r=johannh

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/2835f9a11445